Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cosmic-swingset): add metrics for each action type #10888

Merged
merged 14 commits into from
Jan 28, 2025

Conversation

siarhei-agoric
Copy link
Contributor

@siarhei-agoric siarhei-agoric commented Jan 24, 2025

closes: #10883
refs: #10882

Description

Add metrics for the following action types:

  • CORE_EVAL
  • DELIVER_INBOUND
  • IBC_EVENT
  • INSTALL_BUNDLE
  • PLEASE_PROVISION
  • VBANK_BALANCE_UPDATE
  • WALLET_ACTION
  • WALLET_SPEND_ACTION
  • VTRANSFER_IBC_EVENT
  • KERNEL_UPGRADE_EVENTS

Security Considerations

Metrics may expose inner state which could be used to mount an effective attack.

Scaling Considerations

No impact on a local node performance expected.

Documentation Considerations

Metric names and their meaning need to be documented as now available via standard reporting interface.

Testing Considerations

manual testing?

Upgrade Considerations

This will roll out as part of standard release.

Copy link

cloudflare-workers-and-pages bot commented Jan 24, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6a3eeda
Status: ✅  Deploy successful!
Preview URL: https://429575e2.agoric-sdk.pages.dev
Branch Preview URL: https://sliakh-10883-action-metrics.agoric-sdk.pages.dev

View logs

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Let's try to figure out typing here.

packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
@siarhei-agoric siarhei-agoric marked this pull request as ready for review January 27, 2025 16:54
@siarhei-agoric siarhei-agoric requested a review from a team as a code owner January 27, 2025 16:54
@siarhei-agoric siarhei-agoric force-pushed the sliakh-10883-action-metrics branch from 691e465 to 1cb9440 Compare January 27, 2025 18:39
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to see why we don't need a @ts-expect-error now that we have typing on actions.

packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
@siarhei-agoric
Copy link
Contributor Author

Curious to see why we don't need a @ts-expect-error now that we have typing on actions.

not sure why, but it definitely does not complain:

packages/cosmic-swingset$ yarn lint
yarn run v1.22.19
$ run-s --continue-on-error lint:*
$ tsc
$ eslint .
Done in 34.26s.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is blind to cosmos messages that aren't forwarded to cosmic-swingset. If the goal is a detailed cosmic-swingset perspective, then I think we should merge this metrics data into #10904 and report "cosmic_swingset_inbound_queue" data with both "queue" and "type" dimensional attributes.

packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
@mhofman
Copy link
Member

mhofman commented Jan 28, 2025

This PR is blind to cosmos messages that aren't forwarded to cosmic-swingset

I'm not sure what you mean here. Processed queued actions are indeed specific to messages that made it to cosmic-swingset in the first place through one of the queues.

we should merge this metrics data into #10904 and report "cosmic_swingset_inbound_queue" data with both "queue" and "type" dimensional attributes.

While it's true that any action removed from one of the queue will be processed and accounted for here, I'm not sure we need to merge these 2 things. The type doesn't make much sense to track when decrementing the queue sizes because we never tracked the type when incrementing the queue size. Also, I'm not sure there is much benefit in tracking which queue a processed action came from.

packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
packages/cosmic-swingset/src/launch-chain.js Outdated Show resolved Hide resolved
@siarhei-agoric siarhei-agoric added the automerge:rebase Automatically rebase updates, then merge label Jan 28, 2025
refs: #10883

Move the following action types into QueuedActionType definition:

  CALCULATE_FEES_IN_BEANS: 'CALCULATE_FEES_IN_BEANS',
  VTRANSFER_IBC_EVENT: 'VTRANSFER_IBC_EVENT',
  KERNEL_UPGRADE_EVENTS: 'KERNEL_UPGRADE_EVENTS',
siarhei-agoric and others added 11 commits January 28, 2025 19:02
refs: #10883

Add metrics for the follwoing action types:

  CALCULATE_FEES_IN_BEANS: 'CALCULATE_FEES_IN_BEANS',
  VTRANSFER_IBC_EVENT: 'VTRANSFER_IBC_EVENT',
  KERNEL_UPGRADE_EVENTS: 'KERNEL_UPGRADE_EVENTS',
refs: #10883

launch-chain.js: Generate actionMetrics dynamically from QueuedActionType
action-types.js: Remove unused CALCULATE_FEES_IN_BEANS action type
Co-authored-by: Richard Gibson <[email protected]>
@siarhei-agoric siarhei-agoric force-pushed the sliakh-10883-action-metrics branch from 1fa0ebf to 6a3eeda Compare January 28, 2025 19:03
@siarhei-agoric siarhei-agoric added automerge:squash Automatically squash merge and removed automerge:rebase Automatically rebase updates, then merge labels Jan 28, 2025
@mergify mergify bot merged commit 618553b into master Jan 28, 2025
94 of 103 checks passed
@mergify mergify bot deleted the sliakh-10883-action-metrics branch January 28, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement per-type processed message count for SwingSet
3 participants